-
Notifications
You must be signed in to change notification settings - Fork 8k
intel_adsp: Introduce debug slot manager #97105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
intel_adsp: Introduce debug slot manager #97105
Conversation
f2d5705
to
fc26683
Compare
Changes since v1:
|
@@ -0,0 +1,157 @@ | |||
/* Copyright(c) 2025 Intel Corporation. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space before "(c)" #97103 :-)
if (ADSP_DW->descs[ADSP_DW_SLOT_COUNT].type == ADSP_DW_SLOT_UNUSED || | ||
ADSP_DW->descs[ADSP_DW_SLOT_COUNT].type == dw_desc->type) { | ||
i = ADSP_DW_SLOT_COUNT; | ||
goto found; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firstly, why do we first check the partial slot? What if that type is already allocated, shouldn't we take it? Secondly, I'd really try hard to only use goto
for error handling. If you really want to avoid adding an extra indentation level, you can make a new function just for finding a slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the client is prepared to use partial slot (which is not really common - only GDB can do that atm) then we should try the partial for it as most of clients cannot deal with partial slot and we run out of them easily.
Hrm, right, if for some reason the module which can handle partial slot got a full one, prior (which should not happen as it should not ask for slots multiple time without releasing it) and other module released the partial one then it could be possible...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it worth over-engineering and complicating a simple lookup function?
Sure, I can introduce a bool found = false;
and play with just to avoid the goto, but extra if
s and indentation just makes the code less readable.
I can try to see how it looks..
} | ||
} | ||
|
||
if (ADSP_DW->partial_page0 == slot_address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative: bool found = false;
at the top of the function, found = true; break;
in line 141, if (i == WIN2_SLOTS && ADSP_DW->partial_page0 == slot_address) {
here. Then found = true;
in line 148. Then if (!found) return;
in line 151.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I must then:
void adsp_dw_release_slot(void *slot_address)
{
bool found = false;
int i;
if (ADSP_DW->partial_page0 == slot_address) {
i = ADSP_DW_SLOT_COUNT;
LOG_DBG("Releasing partial page0 used by %#x", ADSP_DW->descs[i].type);
found = true;
} else {
for (i = 0; i < ADSP_DW_FULL_SLOTS; i++) {
if (ADSP_DW->slots[i] == slot_address) {
LOG_DBG("Releasing debug slot %d used by %#x", i,
ADSP_DW->descs[i].type);
found = true;
break;
}
}
}
if (found) {
ADSP_DW->descs[i].resource_id = 0;
ADSP_DW->descs[i].type = ADSP_DW_SLOT_UNUSED;
ADSP_DW->descs[i].vma = 0;
}
}
static uint32_t mem_wptr; | ||
|
||
#ifdef CONFIG_INTEL_ADSP_DEBUG_SLOT_MANAGER | ||
static void *slot_addr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is certainly correct that being static
this variable is only visible in this file, still I'd give it a more descriptive name for easier searching like coredump_slot_addr
or similar. Same applies to other files below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
} __packed; | ||
|
||
#define WIN2_MBASE DT_REG_ADDR(DT_PHANDLE(DT_NODELABEL(mem_window2), memory)) | ||
#define WIN2_SLOTS ((HP_SRAM_WIN2_SIZE / ADSP_DW_PAGE_SIZE) - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we limit this to ADSP_DW_SLOT_COUNT
or to ADSP_DW_SLOT_COUNT - 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define ADSP_DW_FULL_SLOTS MIN(WIN2_SLOTS, ADSP_DW_SLOT_COUNT - 1)
fc26683
to
e07e056
Compare
Changes since v2:
|
|
||
#define WIN2_MBASE DT_REG_ADDR(DT_PHANDLE(DT_NODELABEL(mem_window2), memory)) | ||
#define WIN2_SLOTS ((HP_SRAM_WIN2_SIZE / ADSP_DW_PAGE_SIZE) - 1) | ||
#define ADSP_DW_FULL_SLOTS MIN(WIN2_SLOTS, ADSP_DW_SLOT_COUNT - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ADSP_DW_SLOT_COUNT - 1 should be ``ADSP_DW_SLOT_COUNT
.
The current default of 8192 will provide only 2 pages in debug window: page0: descriptors page1: slot0 However, the coredump is hardwired to use slot1, which by default is not valid. Increase the default window size to 12288 to allow three pages. This change affects CAVS25 only as it is using default window sizes and the window 3 is not used in this configuration at all (it was used with IPC3 only), so we do have enough space for the three page - we could even increase the default to cover 4 pages (8192+8192), but let's be conservative on this. Signed-off-by: Peter Ujfalusi <[email protected]>
Resize the reserved part of the debug window to cover the first 1K and define the partial_page0 which will cover the partial slot in page 0. Add comment to describe the debug window layout. Signed-off-by: Peter Ujfalusi <[email protected]>
Instead of using hardwired index for the shell debug slot, look it up by it's type. Signed-off-by: Peter Ujfalusi <[email protected]>
Currently all drivers which uses a slot from the debug window have fragile hardwired slot 'mapping', they are locked to use specific slots even if there are free slots available for them to take. The new API hides the management of the slots and descriptors and users can ask, release or even seize slots that they want to use. Add a new debug slot manager API and a new default no config option to allow selection between the hardwired or dynamic debug slot management. Signed-off-by: Peter Ujfalusi <[email protected]>
e07e056
to
c19fc77
Compare
Changes since v3:
|
|
ADSP_DW_DESC_COUNT * sizeof(struct adsp_dw_desc)]; | ||
uint8_t partial_page0[ADSP_DW_SLOT_SIZE - ADSP_DW_PAGE0_SLOT_OFFSET]; | ||
uint8_t slots[ADSP_DW_SLOT_COUNT][ADSP_DW_SLOT_SIZE]; | ||
} __packed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confused - you already have this structure in adsp_debug_window.h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is only there if the CONF_INTEL_ADSP_DEBUG_SLOT_MANAGER
is not enabled.
No one should have any knowledge on how the debug window is partitioned, they just ask for a slot and use the slot which is assigned to them, that's all they need to know.
In other words: when the slot manager is enabled I want to make sure that no one can side step and hack into the system.
if (ADSP_DW->descs[ADSP_DW_SLOT_COUNT].type == type) { | ||
return ADSP_DW_SLOT_COUNT; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I understand it correctly, that this function only returns a valid slot index if it already has the desired type? If so, then why the preference? We should never have 2 slots with the same type, so shouldn't matter what the preference is. Just search them all. I don't see a need for that parameter here at all.
Update: oh, I see, this function is also used to find an "unused" slot... That's where you need the preference. This function is called 3 times: twice to find an already allocated slot and once to find a free one. Maybe we could make looking for the existing one a separate function with no preference - even with braces with each if
and for
it should be like 10 lines of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why would we want another function when we can use this? When looking for occupied slot with a given type, yes the preference does not matter, but we eventually still need to check the full slots and the partial anyways.
Currently the debug slots are 'allocated' in wild-west style, no real rules enforced, one has to know exactly which module uses what slots (in hard-wired fashion mostly) which can lead to races and bugs that slots have multiple users and at the same time we still have empty, unused slot(s).
The debug slot manager provides a way for drivers to request a debug slot for their use, or (in case of panic handler) forcibly overtake a given slot.
The debug slot manager can be enabled with a Kconfig change to avoid breakage in compilation - SOF also have modules which tampers with the descriptor page and slots directly.